New: Add config migrations mechanism #648
Conversation
WalkthroughThis change set adds a configuration versioning feature across the codebase. The Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant CM as ConfigManager
participant CR as ConfigMigrator
participant M1 as migrateV0ToV1
C ->> CM: Request configuration
CM ->> CM: Parse configuration
CM ->> CR: Call migrateConfig(parsedConfig)
CR ->> CR: Determine configuration version (getConfigVersion)
alt Configuration is version 0
CR ->> M1: Migrate configuration to V1
end
CR ->> CM: Return migrated configuration
CM ->> C: Return configuration
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/preload/src/commonTypes.ts (1)
17-33:⚠️ Potential issueRemove duplicate Config interface definition.
The file contains two identical definitions of the Config interface. This duplication could lead to maintenance issues and inconsistencies.
Keep the first definition (lines 17-33) and remove the duplicate (lines 221-237).
Also applies to: 221-237
🧹 Nitpick comments (4)
packages/main/src/backend/configManager/configMigration/configMigrator.ts (2)
8-10: Avoid usinganyin themigrationsrecord.
Consider providing a stricter type for both the input and output of each migration function to improve type safety and clarity.-const migrations: Record<number, (config: any) => any> = {}; +import type { ZodTypeAny } from 'zod'; +const migrations: Record<number, (config: unknown) => unknown> = {};
12-26: Consider handling parsing errors more gracefully.
Currently, iflatestConfigSchema.parse()throws, the error will bubble up. This may be acceptable, but wrapping it in user-friendly error logging could aid troubleshooting.packages/main/src/backend/configManager/configMigration/versions/original.ts (2)
31-31: Consider replacinganytype with more specific Zod schemas.The use of
z.any()forcredentialsandloginFieldsfields reduces type safety. Consider defining specific schemas for these fields to ensure proper validation.For the
credentialsfield, consider:- credentials: z.any(), + credentials: z.object({ + access_token: z.string(), + refresh_token: z.string().optional(), + scope: z.string(), + token_type: z.string(), + expiry_date: z.number().optional() + }),For the
loginFieldsfield, consider:- loginFields: z.any(), + loginFields: z.record(z.string(), z.string()),Also applies to: 71-71
85-89: Consider adding a default version field to the original schema.Since this schema represents the original configuration format, it might be helpful to explicitly set a version (e.g., 0) to distinguish it from migrated configurations.
export const originalConfigSchema = z.object({ + version: z.literal(0), outputVendors: outputVendorsSchema, scraping: scrapingSchema, useReactUI: z.boolean().optional(), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
package.json(1 hunks)packages/main/src/backend/commonTypes.ts(1 hunks)packages/main/src/backend/configManager/configManager.ts(2 hunks)packages/main/src/backend/configManager/configMigration/configMigrator.ts(1 hunks)packages/main/src/backend/configManager/configMigration/versions/original.ts(1 hunks)packages/main/src/backend/configManager/configMigration/versions/v1.ts(1 hunks)packages/main/src/backend/configManager/defaultConfig.ts(1 hunks)packages/preload/src/commonTypes.ts(1 hunks)packages/renderer/src/store/Store.test.tsx(1 hunks)packages/renderer/src/types.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/renderer/src/store/Store.test.tsx
- packages/main/src/backend/configManager/defaultConfig.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-13)
🔇 Additional comments (13)
packages/main/src/backend/configManager/configMigration/configMigrator.ts (2)
1-4: Imports look well-organized.
All imported modules are used consistently and align with the intended functionality.
28-31: Version extraction logic is clear and concise.
Using Zod to validate and parse the version ensures reliable migration sequencing.packages/main/src/backend/configManager/configMigration/versions/v1.ts (3)
1-2: Zod import and reference to the original schema look good.
This ensures schema extensions remain type-safe and well-structured.
4-4: Extending the original schema with a literal version is a robust approach.
This design clearly declares the explicit version of the config.
6-11: Migration function neatly upgrades the config from original to v1.
This is straightforward and aligns with best practices for incremental schema evolution.packages/main/src/backend/configManager/configManager.ts (2)
5-5: ImportingmigrateConfighere ensures migrations are applied consistently.
This central point of use is an appropriate place to manage configuration version evolution.
23-23: ApplyingmigrateConfigbefore returning ensures the user always receives a valid config.
The approach integrates well with the surrounding fallback logic.packages/main/src/backend/configManager/configMigration/versions/original.ts (1)
75-83: LGTM! Well-structured scraping configuration schema.The schema properly defines required and optional fields, with appropriate types for each configuration option.
packages/main/src/backend/commonTypes.ts (1)
14-14: LGTM! Version field properly added to Config interface.The addition of the version field is well-typed and aligns with the PR objectives for configuration versioning.
packages/renderer/src/types.tsx (2)
20-20: LGTM! Version field consistently added across type definitions.The version field addition maintains consistency with the Config interface definitions in other files.
14-14:✅ Verification successful
Verify version field usage across the codebase.
Let's ensure all configuration objects include the new version field.
Also applies to: 18-18, 20-20
🏁 Script executed:
#!/bin/bash # Search for Config type usage and object definitions echo "Searching for Config type usage:" rg -p "Config.*?{" -A 5 echo "Searching for configuration object definitions:" rg -p "const.*Config.*?=.*?{" -A 10Length of output: 433
I'll generate a new shell script that uses corrected regular expressions for Ripgrep to re-run the verification of Config interface and object definitions. This will help ensure that we accurately identify configuration objects that include the new version field.
Please run the following script to collect the necessary output:
Once you verify the output and ensure that all configuration objects include the version field, please update the review comment accordingly.
🏁 Script executed:
#!/bin/bash # Corrected regex search for Config interface definitions including the version field. echo "Searching for Config interface definitions with version field:" rg --color=always -n -e "Config\s*{[^}]*version\s*:" # Corrected regex search for configuration object definitions including the version field. echo "Searching for configuration object definitions with version field:" rg --color=always -n -e "const\s+.*Config\s*=\s*{[^}]*version\s*:"Length of output: 667
Please run the following revised script, which escapes the curly braces correctly. This new version should avoid the regex errors and help gather the necessary output:
After running this, please verify the outputs and ensure that all configuration objects include the version field.
🏁 Script executed:
#!/bin/bash # Corrected regex search for Config interface definitions including the version field. echo "Searching for Config interface definitions with version field:" rg --color=always -n -e 'Config\s*\\{[^}]*version\s*:' # Corrected regex search for configuration object definitions including the version field. echo "Searching for configuration object definitions with version field:" rg --color=always -n -e 'const\s+.*Config\s*=\s*\\{[^}]*version\s*:'Length of output: 679
Please run the following updated verification script. It uses fixed‑string searches with context so you can manually inspect the output and confirm that every Config interface or configuration object includes the new version field:
After running this script, manually verify that each Config block (for example, in file
packages/renderer/src/types.tsxat lines 14, 18, and 20) properly includes the version field.Verify that every configuration interface/object includes the new
versionproperty.
🏁 Script executed:
#!/bin/bash echo "=== Searching for occurrences of 'Config {' with context (possible interface blocks) ===" rg -n -F "Config {" --context 5 . echo "" echo "=== Searching for occurrences of 'version:' with context in files ===" rg -n -F "version:" --context 5 .Length of output: 16505
Version Field Verification Completed
After manually verifying the occurrences across the codebase, all Config interfaces and configuration objects consistently include the new
versionfield. This can be seen in the following files:
- packages/main/src/backend/commonTypes.ts — Config interface has
version: number;- packages/renderer/src/types.tsx — Config interface includes
version: number;- packages/preload/src/commonTypes.ts — Config interface shows
version: number;- Additionally, default configuration objects (e.g., in
defaultConfig.ts) correctly define the version property.No further changes are necessary.
package.json (2)
101-102: Added new dependency: Zod (^3.24.2).
This new dependency is essential to support the configuration migration mechanism using Zod for schema validation. Please ensure that the introduced version provides all required features and is tested against expected configuration structures.
108-108: Introduced the packageManager field.
The addition of the"packageManager"field helps to maintain consistency in development environments by explicitly specifying the package manager and its version. Verify that this configuration aligns with the project's tooling expectations and update documentation if necessary.
| // migrations[n] should be a function that converts version n to version n+1 | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const migrations: Record<number, (config: any) => any> = {}; |
There was a problem hiding this comment.
Since migrations is gonna hold all migration functions, and each migration function has a different signature (v1 => v2, v2 => v3, etc...), I'm not sure if it's possible to type this correctly :/
I tried experimenting with z.discriminatedUnion('version', [...]), or defining a shared interface interface VersionedConfig {version: number}, but I couldn't get anything to work. If you can think of a way to avoid any here please let me know 🙂
whatuserever
force-pushed
the
config_migrations
branch
from
486cda1 to
304e55e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/main/src/backend/configManager/configMigration/versions/original.ts (3)
5-26: Standardize enum value casing for consistency.The
companyTypeSchemaenum values use inconsistent casing patterns. Consider standardizing to camelCase for all values.Apply this diff to standardize the casing:
export const companyTypeSchema = z.enum([ 'hapoalim', 'hapoalimBeOnline', 'beinleumi', - 'union', + 'unionBank', 'amex', 'isracard', 'visaCal', 'max', - 'leumiCard', + 'leumicard', - 'otsarHahayal', + 'otsarHaHayal', 'discount', 'mercantile', 'mizrahi', 'leumi', 'massad', 'yahav', - 'behatsdaa', + 'beHatsdaa', - 'beyahadBishvilha', + 'beYahadBishvilha', 'oneZero', 'pagi', ]);
75-83: Add documentation and defaults for configuration fields.The
scrapingSchemawould benefit from:
- Documentation about the unit of measurement for the
timeoutfield (seconds/milliseconds)- A default value for
maxConcurrencyto prevent potential performance issuesConsider adding defaults and refinements:
export const scrapingSchema = z.object({ numDaysBack: z.number(), showBrowser: z.boolean(), accountsToScrape: z.array(accountToScrapeConfigSchema), chromiumPath: z.string().optional(), - maxConcurrency: z.number().optional(), + maxConcurrency: z.number().optional().default(2), - timeout: z.number(), + timeout: z.number().describe('Timeout in milliseconds'), periodicScrapingIntervalHours: z.number().optional(), });
91-94: Add tests for the configuration type guard.The
isOriginalConfigtype guard is crucial for the migration mechanism. Please add unit tests to verify its behavior with valid and invalid configurations.Would you like me to help generate test cases for the type guard function? Here's an example of what the tests could look like:
describe('isOriginalConfig', () => { it('should return true for valid config', () => { const validConfig = { outputVendors: { json: { active: true, options: { filePath: '/tmp/output.json' } } }, scraping: { numDaysBack: 30, showBrowser: false, accountsToScrape: [], timeout: 30000 } }; expect(isOriginalConfig(validConfig)).toBe(true); }); it('should return false for invalid config', () => { const invalidConfig = { outputVendors: { invalid: { active: true } } }; expect(isOriginalConfig(invalidConfig)).toBe(false); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
package.json(1 hunks)packages/main/src/backend/commonTypes.ts(1 hunks)packages/main/src/backend/configManager/configManager.ts(2 hunks)packages/main/src/backend/configManager/configMigration/configMigrator.ts(1 hunks)packages/main/src/backend/configManager/configMigration/versions/original.ts(1 hunks)packages/main/src/backend/configManager/configMigration/versions/v1.ts(1 hunks)packages/main/src/backend/configManager/defaultConfig.ts(1 hunks)packages/preload/src/commonTypes.ts(1 hunks)packages/renderer/src/store/Store.test.tsx(1 hunks)packages/renderer/src/types.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/main/src/backend/commonTypes.ts
- packages/renderer/src/store/Store.test.tsx
- packages/main/src/backend/configManager/defaultConfig.ts
- packages/main/src/backend/configManager/configManager.ts
- packages/renderer/src/types.tsx
- package.json
- packages/main/src/backend/configManager/configMigration/versions/v1.ts
- packages/main/src/backend/configManager/configMigration/configMigrator.ts
- packages/preload/src/commonTypes.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-13)
🔇 Additional comments (1)
packages/main/src/backend/configManager/configMigration/versions/original.ts (1)
60-65: Well-structured vendor schema implementation!The
outputVendorsSchemaeffectively combines all vendor schemas using computed property names, making each vendor configuration optional. This provides good flexibility while maintaining type safety.
| export const googleSheetsConfigSchema = z.object({ | ||
| active: z.boolean(), | ||
| options: z.object({ | ||
| credentials: z.any(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any types with more specific types.
The use of any in credentials and loginFields bypasses TypeScript's type checking. Consider defining specific types or interfaces for these fields to maintain type safety.
For credentials, consider using the Google OAuth2 credentials type:
type GoogleCredentials = {
client_email: string;
private_key: string;
[key: string]: string;
};For loginFields, consider defining a union type of possible login field configurations:
type LoginField = {
type: 'text' | 'password';
name: string;
value: string;
};
type LoginFields = {
[key: string]: LoginField;
};Also applies to: 71-71
There was a problem hiding this comment.
credentials and loginFields are coming from 3rd parties (Google, israeli-bank-scrapers). I didn't want to create a schema for them as they're outside of our control.
There was a problem hiding this comment.
@whatuserever Understood, that's a valid approach for handling third-party data structures that are outside of our control. I'll retract my suggestion about replacing these specific any types.
✏️ Learnings added
Learnt from: whatuserever
PR: brafdlog/caspion#648
File: packages/main/src/backend/configManager/configMigration/versions/original.ts:31-31
Timestamp: 2025-02-16T12:10:12.122Z
Learning: In the Caspion project, `any` type is intentionally used for third-party data structures (like `credentials` from Google and `loginFields` from israeli-bank-scrapers) that are outside of our control to avoid maintenance overhead and potential breakage when third-party structures change.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| return { | ||
| ...v1Config, | ||
| version: 1, | ||
| }; |
There was a problem hiding this comment.
Exactly, this is the migration:
const migrated = {
...originalConfig,
version: 1
}
You can do it without all the previous zod preparations.
There was a problem hiding this comment.
The preperation is needed to create a snapshot of the config interface that is decoupled from the Config type used in the "live" code. Otherwise, how can you write a function that accepts version n and returns version n+1, while the live code is at the latest version m?
Maybe looking at what I did for v2 will help clarify that? You can check it out here (I didn't update #647 yet as I'm waiting for this PR to get approved).
We can do this preperation by copying the types instead of using Zod, but we still need it. And in my opinion Zod is a better fit for this task as it lets you safely parse the input file, allowing you to verify the version before applying any migrations.
There was a problem hiding this comment.
Alright, removed all the zod stuff and used a simple any for all migration code
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/main/src/backend/configManager/configMigration/versions/v1.ts (2)
3-4: Consider adding type safety when possibleWhile using
anytype is currently necessary based on the past discussions about decoupling from live code types, consider documenting the expected shape ofv0Configin a comment to improve maintainability.// eslint-disable-next-line @typescript-eslint/no-explicit-any +// v0Config is expected to be the original config object without version property export function migrateV0ToV1(v0Config: any): any {
1-9: Consider adding validation and error handlingThe current implementation assumes the input is a valid configuration object. Consider adding basic validation to ensure the input is at least an object before attempting migration.
// eslint-disable-next-line @typescript-eslint/no-explicit-any export function migrateV0ToV1(v0Config: any): any { + if (!v0Config || typeof v0Config !== 'object' || Array.isArray(v0Config)) { + throw new Error('Invalid config: Expected an object'); + } return { ...v0Config, version: 1, }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/main/src/backend/configManager/configMigration/configMigrator.ts(1 hunks)packages/main/src/backend/configManager/configMigration/versions/v1.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/main/src/backend/configManager/configMigration/configMigrator.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (macos-13)
- GitHub Check: build (windows-latest)
🔇 Additional comments (2)
packages/main/src/backend/configManager/configMigration/versions/v1.ts (2)
1-2: Clear comment that explains the purpose of this migrationThe comment succinctly describes what this migration accomplishes - adding the version field to the config object. This helps developers understand the purpose of this file at a glance.
5-9: Simple and effective migration implementationThe migration logic is straightforward and accomplishes exactly what's described in the PR objectives - preserving existing configuration values while adding the new version field.
| // V1 adds the version field to the config object | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| export function migrateV0ToV1(v0Config: any): any { | ||
| return { | ||
| ...v0Config, | ||
| version: 1, | ||
| }; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Remember to add tests for this migration function
As noted in the PR objectives, tests for the migration mechanism are marked as incomplete. Ensure tests are added to verify this migration function works correctly with various input configurations.
🏁 Script executed:
#!/bin/bash
# Check if there are any test files for migrations
fd -t f "test|spec" --exec grep -l "migrateV0ToV1" {}Length of output: 55
Action Required: Add Tests for the Migration Function
The migration function in packages/main/src/backend/configManager/configMigration/versions/v1.ts currently lacks test coverage. Our verification (via the search for occurrences of migrateV0ToV1 in test files) confirms no tests are present for this function. Please add tests to cover various scenarios with differing input configurations to ensure the migration behaves correctly.
- File to address:
packages/main/src/backend/configManager/configMigration/versions/v1.ts - Action: Create tests (e.g., in your test/spec directory) that verify:
- The function correctly adds the
version: 1field. - It properly handles different forms of input (e.g., undefined or unexpected values).
- The function correctly adds the
There was a problem hiding this comment.
Eh. As @baruchiro pointed out, these migrations are written once and shouldn't be modified again. This is why we're ok with it being untyped (any). By that same logic, I don't see the need for tests.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Why does this even happen?!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/main/src/backend/configManager/configMigration/migrations/v1.ts (2)
3-4: Consider type safety improvements for future migrations.While using
anytype is mentioned in the PR objectives as an intentional design choice, consider documenting why this approach was chosen over typed schemas for better maintainability. For future migrations where transformations might be more complex, this could help prevent errors.
5-9: Looks good, but consider adding validation.The migration function correctly preserves existing configuration while adding the version field. However, consider adding validation to ensure the input is actually a valid configuration object.
export function migrateV0ToV1(v0Config: any): any { + // Basic validation to ensure we're working with an object + if (!v0Config || typeof v0Config !== 'object') { + throw new Error('Invalid configuration: expected an object'); + } + return { ...v0Config, version: 1, }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/main/src/backend/configManager/configMigration/configMigrator.ts(1 hunks)packages/main/src/backend/configManager/configMigration/migrations/v1.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/main/src/backend/configManager/configMigration/configMigrator.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (macos-13)
- GitHub Check: build (windows-latest)
🔇 Additional comments (2)
packages/main/src/backend/configManager/configMigration/migrations/v1.ts (2)
1-2: Good descriptive comment.The comment clearly explains the purpose of this migration - adding the version field to the config object.
1-9: Remember to add tests as mentioned in PR objectives.The PR objectives mentioned that tests for the migration mechanism are incomplete. Make sure to add tests that verify this migration function works correctly with various input configurations.
You could create test cases for:
- Valid configuration objects
- Empty configuration objects
- Handling edge cases (null, undefined)
#647 makes a breaking change to the config. To avoid breaking installations with the old config, @baruchiro suggested implementing a migration mechanism.
This PR implements it using Zod:Defined Zod schema for each config version. This avoids creating a dependency on the types used in the "live" code.Usedanyfor the config types.nand return a config of versionn+1versionkey to theConfigtype.Summary by CodeRabbit